Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add uhid service and alternative cli #35

Merged
merged 13 commits into from
May 1, 2023

Conversation

felfert
Copy link
Collaborator

@felfert felfert commented Apr 26, 2023

Description

As announced in #22 , I implemented a uhid driver service together with an alternative cli that acts as a drop-in replacement for cherryrgb_cli. See docs/UHID-driver.md.

Fixes # 22

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Adds/updates Documentation

Code Checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have implemented respective test(s)
  • I have run lints and tests (cargo fmt && cargo clippy && cargo test) that prove my fix is effective or that my feature works

@felfert
Copy link
Collaborator Author

felfert commented Apr 26, 2023

Strange, that all except fmt failed. builds fine here?!

@felfert
Copy link
Collaborator Author

felfert commented Apr 27, 2023

Looks like the build system is missing llvm-config and libclang:

2023-04-26T20:10:00.8926365Z warning: could not execute `llvm-config` one or more times, if the LLVM_CONFIG_PATH environment variable is set to a full path to valid `llvm-config` executable it will be used to try to find an instance of `libclang` on your system: "couldn't execute `llvm-config --prefix` (path=llvm-config) (error: No such file or directory (os error 2))"
2023-04-26T20:10:00.8928739Z 
2023-04-26T20:10:00.8929255Z error: failed to run custom build command for `clang-sys v1.6.1`
2023-04-26T20:10:00.8929867Z 
2023-04-26T20:10:00.8930107Z Caused by:
2023-04-26T20:10:00.8931102Z   process didn't exit successfully: `/home/runner/work/cherryrgb-rs/cherryrgb-rs/target/debug/build/clang-sys-dad454f8793fe135/build-script-build` (exit status: 101)
2023-04-26T20:10:00.8931743Z   --- stdout
2023-04-26T20:10:00.8933133Z   cargo:warning=could not execute `llvm-config` one or more times, if the LLVM_CONFIG_PATH environment variable is set to a full path to valid `llvm-config` executable it will be used to try to find an instance of `libclang` on your system: "couldn't execute `llvm-config --prefix` (path=llvm-config) (error: No such file or directory (os error 2))"
2023-04-26T20:10:00.8933853Z 
2023-04-26T20:10:00.8934329Z   --- stderr
2023-04-26T20:10:00.8951960Z   thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: "couldn't find any valid shared libraries matching: ['libclang.so', 'libclang-*.so'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"', /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clang-sys-1.6.1/build/dynamic.rs:206:45
2023-04-26T20:10:00.8954113Z   note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2023-04-26T20:10:00.8954661Z warning: build failed, waiting for other jobs to finish...
2023-04-26T20:10:01.1370052Z ##[error]Process completed with exit code 101.
2023-04-26T20:10:01.1466111Z Post job cleanup.

This new dependency is introduced indirectly by uhid-virt:

   └── uhid-virt v0.0.6
│       ├── enumflags2 v0.7.7
│       │   └── enumflags2_derive v0.7.7 (proc-macro)
│       │       ├── proc-macro2 v1.0.56 (*)
│       │       ├── quote v1.0.26 (*)
│       │       └── syn v2.0.15 (*)
│       ├── libc v0.2.142
│       └── uhidrs-sys v1.0.2
│           [build-dependencies]
│           └── bindgen v0.63.0
│               ├── bitflags v1.3.2
│               ├── cexpr v0.6.0
│               │   └── nom v7.1.3
│               │       ├── memchr v2.5.0
│               │       └── minimal-lexical v0.2.1
│               ├── clang-sys v1.6.1

@felfert
Copy link
Collaborator Author

felfert commented Apr 27, 2023

Forget that:

Have a look here:

The step named "Install dependencies" apparently does the trick

clippy:
    name: Clippy
    runs-on: ubuntu-latest
    steps:
      - name: Install dependencies
        run: sudo apt-get update && sudo apt-get install -y libclang-dev clang llvm
      - uses: actions/checkout@v3
      - uses: dtolnay/rust-toolchain@stable
        with:
          components: clippy
      - name: Cargo Clippy
        run: cargo clippy -- -D warnings
      - name: Cargo Clippy Example
        run: cargo clippy --manifest-path example/Cargo.toml -- -D warnings

@skraus-dev
Copy link
Owner

skraus-dev commented Apr 27, 2023

Thanks alot for doing this major task!

Now, to get this merged, there needs to be some conditional compiling happening - so non-linux user can still build the project.

Checklist:

  • Conditionally compile uhid-exclusive functionality in the main lib related to target_os (target_os: linux, reference: https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-attribute)
    • module: vkbd
    • structs: models.RpcAnimation
  • Conditionally include the dependency uhid-virt, only on the linux-platform
  • CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)
  • CI: Ensure correct build command is used per platform (build --all for linux, plain build for the others)
  • CI: Ensure correct binaries and auxilary files (udev rules, service file, config file) are bundled in build artifact / release bundle

With these changes it should already work from what I can tell.

Linux: cargo build --all
Non-Linux: cargo build (it should not be attempting to build everything, aka. only builds the CLI)

@skraus-dev skraus-dev added the enhancement New feature or request label Apr 27, 2023
@felfert
Copy link
Collaborator Author

felfert commented Apr 27, 2023

Thanks alot for doing this major task!

Now, to get this merged, there needs to be some conditional compiling happening - so non-linux user can still build the project.

Checklist:

* [x]  Conditionally compile uhid-exclusive functionality in the main lib related to target_os (target_os: linux, reference: https://doc.rust-lang.org/reference/conditional-compilation.html#the-cfg-attribute)
  
  * [x]  module: vkbd
  * [x]  structs: models.RpcAnimation

* [x]  Conditionally include the dependency `uhid-virt`, only on the linux-platform

Wahh, you are too quick, I have the above already - just not yet committed to this PR's branch

* [ ]  CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)

I tried github's official rust action instead and it works nicely. (not yet tested for i36 and arm)

* [ ]  CI: Ensure correct build command is used per platform (`build --all` for linux, plain `build` for the others)

That's what I am struggling with right now: I cannot believe, that there is NO way of disabling build of specific workspace members based on target-os or features. What a pity! (There are some Issues open in cargo's repo, but no solution exists yet. I experimented with conditionals in both service and ncli, but this is ridiculously ugly. This are 2 very weak spots in cargo and rustc - to say the least.

* [ ]  CI: Ensure correct binaries and auxilary files (udev rules, service file, config file) are bundled in build artifact / release bundle

Huh: Did you mean CD?

With these changes it should already work from what I can tell.

Linux: cargo build --all Non-Linux: cargo build (it should not be attempting to build everything, aka. only builds the CLI)

@felfert
Copy link
Collaborator Author

felfert commented Apr 27, 2023

I think, triggering the CD action on any pull request is NOT a good idea, because - as far as i understand it - the
publishing to releases and cargo runs in YOUR context (using the secrets you provide in your configuration).
Instead, the matrix-build and the copying to an output directory should be moved to the CI action and the resulting
output directory then stashed for later retrieval by the CD action. Then the CD action, just retrieves the previouslly
stashed artifacts and publishes them.

@felfert
Copy link
Collaborator Author

felfert commented Apr 27, 2023

* [ ]  CI: Ensure build dependencies get installed (natively for x86_64, inside cross container for i386, arm)

llvm and clang are capable of cross-compiling per se, so no problem there, but libclang-dev (and possible dependencies)
is going to be tricky, because there are no pre-made packages:

  • a complete cross env would have to be created
  • libclang-dev installed inside of that one

This is a) much bloat, b) quite time-consuming if done in the regular CI action. I will try to generate a separate action
that perfoms this and then stashes the files of the lib for retrieve by the CI action. This might take some time.

@skraus-dev
Copy link
Owner

skraus-dev commented Apr 27, 2023

@felfert no worries, the build dependency task is a tiny one, involving the cross rust toolchain

https://github.com/cross-rs/cross/wiki/Configuration

We just need to decide whether we want a seperate Cross.toml or we put the pre-build dependency installation step inside the existing Cargo.toml

I think, triggering the CD action on any pull request is NOT a good idea, because - as far as i understand it - the publishing to releases and cargo runs in YOUR context (using the secrets you provide in your configuration). Instead, the matrix-build and the copying to an output directory should be moved to the CI action and the resulting output directory then stashed for later retrieval by the CD action. Then the CD action, just retrieves the previouslly stashed artifacts and publishes them.

Kind of like that, yes.
My plan is to have the main build action.. and then I will introduce a seperate actions that uses it as "re-usable workflow" that will only run on "main" and "tags".

reference: https://docs.github.com/en/actions/using-workflows/reusing-workflows

@felfert
Copy link
Collaborator Author

felfert commented Apr 28, 2023

Oh, and can we please keep CI/CD related commits out of this PR. I think, these are better collected in a separate PR.
I'm closing this, and re-submit a new PR without those.

For now, this is what I have No PR yet (still without the cross stuff, but artifacts are already stashed):
https://github.com/felfert/cherryrgb-rs/actions/runs/4830348450

Notes:

  • Intentionally NOT named CI, so we can have this in addition to the existing one. If everything is finalized, we can replace the original CI with the new one.
  • Intentionally only manually triggered, so it does not waste resources all the time.

Question:
Why do you use dtolnay/rust-toolchain@stable instead of the default that comes preinstalled on githubs native platforms?

@felfert
Copy link
Collaborator Author

felfert commented Apr 28, 2023

Ok, after wasting a whole evening for testing research etc, it turns out, that libclang on ubuntu is seriously broken in
regards of multiarch installs. (cross uses ubuntu-16.04 (ancient!) as base-image). Unfortunately, cross also is
not very userfriendly in that it does not show any output of the pre-build comand in Cross.toml. So I had to
resort to manually entering the container locally and figuring out what went wrong:

Basically, the problem is similar like this:
https://askubuntu.com/questions/1446189/problem-doing-multiarch-installs-on-ubuntu
The packages of libclang for different architectures all install to the same location. The native amd64 lib is at
/usr/lib/llvm-3.8/lib/libclang-3.8.so.1 and the arm64 and i686 packages try to overwrite this - which is obviously
fatal:

Snippet from the apt-get install libclang-dev:i386
Unpacking libclang1-3.8:i386 (1:3.8-2ubuntu4) ...
dpkg: error processing archive /var/cache/apt/archives/libclang1-3.8_1%3a3.8-2ubuntu4_i386.deb (--unpack):
trying to overwrite shared '/usr/lib/llvm-3.8/lib/libclang-3.8.so.1', which is different from other instances of package libclang1-3.8:i386

Same happens with arm64

The correct path would be somewhere below /usr/lib/i386-linux-gnu/ resp. /usr/lib/arm64-linux-gnu/

So unless you know of a different container for cross-builds, I give up on that. It's simply not worth the effort,
especially since both architectures are not really widely used.

Well one last try would be manually unpacking the .deb in temporary dir and then moving it to a different location.
Given that ubuntu 16.04 is ancient, I dont even think about bug report.

@felfert felfert force-pushed the uhid-service branch 2 times, most recently from 05e64d1 to 04c0eb6 Compare April 29, 2023 05:32
@felfert
Copy link
Collaborator Author

felfert commented Apr 29, 2023

So for now, I have added a featue named uhid and changed the conditional compile directives.
In order to build, both target_os must be "linux" AND feature must be "uhid"
So in the matrix CI action we use --all --features uhid on native ubuntu latest only

If the libclang mess gets sorted out for the cross platforms, we can add these flags for them later.

The new CI is ready, (see here). Currently working on new CD. PR for both coming soon...

Cheers
-Fritz

@felfert
Copy link
Collaborator Author

felfert commented Apr 29, 2023

Turns out, artifacts can only be shared in the same Action. So switching to cache. Keeping the artifact for manual checks, if eferything is in order.

@felfert
Copy link
Collaborator Author

felfert commented Apr 30, 2023

Turns out, artifacts can only be shared in the same Action. So switching to cache. Keeping the artifact for manual checks, if eferything is in order.

Also, the github deployment action insists being triggered by a git tag event which makes my idea of how this should work completely useless. I must admit, I never had used github workflow/actions before. Professionally, I am used to a jenkins environment which is quite different in that perspective and I was a probably a bit naive in assuming that the github stuff works in a similar way.

So: PR coming up with fixes for the existing workflows ...

felfert added a commit to felfert/cherryrgb-rs that referenced this pull request Apr 30, 2023
- Install build dependencies when building the UHID stuff.
- Build UHID stuff on native linux only for now.
@felfert felfert mentioned this pull request Apr 30, 2023
1 task
@skraus-dev skraus-dev merged commit 8da0d14 into skraus-dev:master May 1, 2023
@felfert felfert deleted the uhid-service branch May 10, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants